Repository navigation
Conversation
TrickyPi
changed the title
|
@greptile review |
 [Medium impact] The PR appears safe to merge; no blocking issues remain. Reviews (4) · Last reviewed commit: "Merge branch 'master' into feat/rollup-5..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
lukastaegert
left a comment
lukastaegert
left a comment
There was a problem hiding this comment.
Hi, thank you for taking care! I had a look at the changes and the fix looks good, however it would be nice to extend the testing a little more.
The pre-release version of rollup@5 is now basically complete, so we could switch the json plugin to directly test with this version. Maybe you can do it like #2037, #2038, #2039 by defining a catalog version? Then all other update PRs can also use that.
| test('excludes json files with import attributes', async () => { | ||
| expect( | ||
| json({ exclude: /\.json$/ }).transform(`{"a":"b"}`, idWithJsonAttributes, { | ||
| rawId: 'input.json' | ||
| }) | ||
| ).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
This test currently does not trigger out fix because it filters out the file. What about this
| test('excludes json files with import attributes', async () => { | |
| expect( | |
| json({ exclude: /\.json$/ }).transform(`{"a":"b"}`, idWithJsonAttributes, { | |
| rawId: 'input.json' | |
| }) | |
| ).toBeNull(); | |
| }); | |
| test('applies exclude patterns to the raw id instead of the encoded id', async () => { | |
| expect( | |
| json({ exclude: /attributes/ }).transform(`{"a":"b"}`, idWithJsonAttributes, { | |
| rawId: 'input.json' | |
| }).code | |
| ).toEqual('export var a = "b";\nexport default {\n\ta: a\n};\n'); | |
| }); |
Rollup Plugin Name:
jsonThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
related to rollup/rollup#6266
Description
Rollup 5 will support importing the same module with different import attributes. As a result, the
idpassed as the second argument to thetransformhook may include encoded import attributes, causing the existingid.slice(-5) !== '.json'check to incorrectly skip JSON modules.This PR uses
transformOptions.rawIdfor the JSON extension check when it is available and falls back toidfor compatibility with earlier Rollup versions.